fix(approver): allow delta pose modes with clampable rotation deltas#144
fix(approver): allow delta pose modes with clampable rotation deltas#144Tushar2604 wants to merge 2 commits into
Conversation
jeqcho
left a comment
jeqcho
left a comment
There was a problem hiding this comment.
Thanks Tushar. The test work here is the strong part: the conformance-level test pins the actual symptom from #143 (doctor reporting a BridgeData-style 7-D xyz+euler embodiment non-conformant) rather than just the constructor, and scoping the fix to the constructor gate while leaving review() untouched is the right cut. Defining _ABSOLUTE_POSE_MODES as an intersection so it self-maintains is a nice touch too.
One thing blocks it: the premise "rotation deltas clamp per dimension like any other bounded displacement" holds for euler_xyz and axis_angle, but not for quaternions. A no-op quat delta is (1, 0, 0, 0), not the zero vector. Clamp w into a symmetric ±max_delta box and any downstream normalization inflates the rotation instead of limiting it. Concretely: an intended delta of (1, 0.05, 0.05, 0.05) is about a 10 degree rotation; with max_delta=0.05 the clamp yields (0.05, 0.05, 0.05, 0.05), which normalizes to w=0.5, a 120 degree rotation. So --max-action-delta, the flag this PR restores, would amplify rotations on a quat-delta embodiment rather than limit them.
Could you keep the refusal for quat_wxyz/quat_xyzw in displacement mode (message along the lines of "quat identity is not at the origin, per-dimension clamping distorts it") and let euler_xyz and axis_angle through? The parametrized test then splits naturally: euler_xyz/axis_angle keep the happy-path clamp assertion, quat_* assert the refusal. Related: the current quat_wxyz case uses shape=(7,), but a quat delta pose is 3 pos + 4 quat + 1 gripper = 8 dims, so as written it pins behavior on a layout that can't exist.
Two smaller notes, take or leave:
quat_xyzwcurrently has no coverage either way; the split above covers it for free.- House style: bold is reserved for definition-list lead-ins, so the mid-sentence
**absolute**in the docstring,adapters.md, and the CHANGELOG body should drop to plain text.
rot6d in displacement mode has the same identity-not-at-origin problem (no-op is (1,0,0,0,1,0)), but that predates this PR, so I'll open a follow-up issue rather than load it onto this one.
The euler-delta unblocking is what #143 needed, happy to merge once the quat gate is back.
|
@jeqcho Thanks for the detailed review — all addressed in 1bcf88e. 1)Restored the quat refusal in displacement mode (quat_wxyz/quat_xyzw), letting euler_xyz/axis_angle through. Self-maintaining via _DISPLACEMENT_POSE_MODES = _POSE_MODES - _ABSOLUTE_POSE_MODES, mirroring the intersection pattern you liked. 2)Fixed the shape bug (quat delta pose is 8 dims, not 7) and split the test: euler_xyz/axis_angle keep the happy-path clamp assertion, quat_wxyz/quat_xyzw get their own parametrized refusal test — quat_xyzw now covered too. 3)Dropped the mid-sentence bold in the docstring, adapters.md, and the CHANGELOG, and updated the surrounding text since it no longer holds that displacement mode accepts any representation. Verified all six mode/rep combinations end-to-end and reran the full suite on Linux: 100% coverage, mypy strict, ruff, mkdocs strict. rot6d's identical problem in displacement mode is still out of scope here per your note — happy to pick up #150 next. |
|
The update looks right, @Tushar2604. I checked the math on the euler_xyz and axis_angle cases: both have their no-op at the origin, so a zero-centered per-dimension clip can only shrink each component, and for axis_angle that means the encoded angle (the vector norm) can only shrink too. No amplification path. Deriving One step left before this can land: the branch now conflicts with main, but only in CHANGELOG.md (#146 and #155 merged entries under Unreleased since you branched). Could you rebase onto main and resolve that one hunk? That will also unstick CI, which GitHub silently refuses to run while a PR has conflicts, so your last two commits have never actually been through the pipeline. Optional while you're in there: the new displacement refusal message hardcodes "euler_xyz and axis_angle are safe here" as prose. Deriving it from the sets like the absolute-mode message does with |
DeltaLimitApprover rejected any pose mode whose rotation_repr was not in
{none, rot6d}, but that guard fired for displacement pose modes too. For
eef_delta_pose the per-dim quantities are small rotation deltas, which clamp
per dimension just like any bounded displacement; only absolute orientations
(eef_abs_pose) have the wraparound and axis-coupling problems the guard
targets.
Restrict the rotation-repr refusal to absolute pose modes. The displacement
review path already clamps every dim through the box, so no runtime change is
needed there. Downstream recovers for free: conformance's guardrail probe now
reports euler-delta embodiments conformant, and CLI runs keep delta limiting
instead of degrading to clamp-only.
Closes robocurve#143.
Address review: the displacement-pose branch had no rotation-repr gate at
all, so quat_wxyz/quat_xyzw slipped through alongside the intended
euler_xyz/axis_angle. A quaternion delta's identity is (1, 0, 0, 0), not
the zero vector, so clamping it per dimension toward a symmetric
±max_delta box drags it away from identity; downstream re-normalization
can then amplify the rotation instead of limiting it (same failure class
as clamping an absolute quaternion).
- Add _DISPLACEMENT_POSE_UNSAFE_ROT = {quat_wxyz, quat_xyzw} and a second
constructor refusal, self-maintaining via _DISPLACEMENT_POSE_MODES =
_POSE_MODES - _ABSOLUTE_POSE_MODES.
- Fix the quat test's shape bug (a quat delta pose is 3 pos + 4 quat + 1
gripper = 8 dims, not 7) and split it into its own parametrized refusal
test over quat_wxyz/quat_xyzw, covering quat_xyzw for the first time.
euler_xyz/axis_angle keep the original happy-path clamp assertion.
- Drop mid-sentence bold in the docstring, adapters.md, and the CHANGELOG
entry, and update their content to describe the quat refusal accurately.
- Derive the displacement refusal message's safe-rep list from
RotationRepr itself (get_args(RotationRepr) - _DISPLACEMENT_POSE_UNSAFE_ROT)
instead of hardcoding "euler_xyz and axis_angle" as prose, so it can't
drift if the sets change (review nit).
rot6d has the same identity-not-at-origin problem in displacement mode but
is intentionally out of scope here (robocurve#150).
1bcf88e to
0611b14
Compare
|
@jeqcho Rebased onto main and resolved the CHANGELOG.md conflict — it was two overlapping additions in the same spot (my two commits both touch that Fixed entry, plus main had picked up #145 and #154 in between), nothing content-related. Also took the optional nit: the displacement refusal message now derives its safe-rep list from get_args(RotationRepr) - _DISPLACEMENT_POSE_UNSAFE_ROT instead of hardcoding "euler_xyz and axis_angle" as prose. Folded it into the same commit that introduced the message rather than adding a third one. Verified on Linux: ruff, format, mypy strict (63 files), 100% coverage, 743 passed. Force-pushed (0611b14) — should be conflict-free and CI should actually run now. |
2 participants
Closes #143
Problem
DeltaLimitApproverraisedValueErrorfor any pose mode whoserotation_reprwasn't in{none, rot6d}. The check fired for_POSE_MODES, which includes the displacement modeeef_delta_pose.For absolute orientations that's sound, since per-dimension clamping of absolute Euler/quaternion representations has wraparound and axis-coupling problems. However, a displacement pose mode's per-dimension quantities are small rotation deltas, which clamp per dimension just fine.
The false rejection had two downstream effects:
inspect-robots-widowx/ BridgeData V2's 7-Dxyz + eulerdeltas).--max-action-deltaunable to restore delta limiting.Fix
Restrict the rotation-representation refusal to absolute pose modes:
The displacement
review()path already clamps every dimension through the box, so no runtime change is needed there.As a result, downstream behavior recovers automatically:
doctornow reports Euler-delta embodiments as conformant.Testing
test_approvers.pyKept the absolute-pose rejection (renamed to explicitly refer to "absolute").
Added a parametrized
eef_delta_pose × {euler_xyz, quat_wxyz, axis_angle}test asserting:±max_deltabox.test_conformance.pyeuler-delta-is-guardrail-conformanttest.Verification
Verified on Linux:
ruffruff formatmypy --strict(62 files)pytest --covat 100% (approver.pyfully covered)Result
Unblocks
inspect-robots-widowxdeclaringeuler_xyzhonestly once released, per the issue.